fix: add stream creation time in get stats api #632

gurjotkaur20 · 2024-01-22T07:05:50Z

Fixes #587

Description

Added stream creation time in the get stats api.

nitisht · 2024-01-22T07:54:59Z

Fixes #574

Is this the correct issue this PR is fixing?

gurjotkaur20 · 2024-01-22T09:08:53Z

Fixes #574

Is this the correct issue this PR is fixing?

My bad. I have corrected the issue number. Plus this PR is adding stream creation timestamp. Let me know if this is sufficient or we need first event timestamp as well?

nitisht · 2024-01-22T09:45:03Z

Thanks @gurjotkaur20 .

Let me know if this is sufficient or we need first event timestamp as well?

Quoting @logut from issue #587,

It would me nice to be able to get the timestamp of the first event of the logstream in the stats in the api and console.

So, let's add the first event timestamp in the API as well.

nitisht · 2024-01-22T09:49:29Z

We can add an additional field in the stream.json file, first-event-at. This would be empty at the stream creation time, but will be updated when the first event is received on the log stream.

Then return this in the get-stats API.

logut · 2024-01-22T11:17:33Z

Thanks for working on this !
If you store statically the first event in the stream.json file, would it be updated when retention cleanup is applied ?
I would be interested more in "first event queryable for the stream" rather than "first event ever"

nitisht · 2024-01-22T13:40:37Z

If you store statically the first event in the stream.json file, would it be updated when retention cleanup is applied ?

Great catch, we'll have to keep updating the stream time. In this case, don't you think it is better to use SQL directly @logut ?

gurjotkaur20 · 2024-01-23T16:36:29Z

@nitisht I've added first event time. Please take a look. However, I've noticed one point. For example,

There is an event and first-event-at is set in the stream.json. It gets cleaned up during retention cleanup. Now, there's no event left in the stream. The stats still shows the event count as well as the old first-event-at time.
Is this the expected behavior?
Or
Should it be handled differently?

nitisht

IMO there is a simpler way, we have something called .stream.json per Stream - this file has a section called snapshot.manifest_list, this points to all manifest.json files (created per day)

To find the first query-able event time stamp, you could see the list snapshot.manifest_list, go to the first entry in this list which would be a manifest.json file, in this manifest.json, you can get the smallest p_timestamp

This approach would need max of two S3 calls and will be much cheaper computationally

nitisht · 2024-02-06T16:01:54Z

@gurjotkaur20 now that #649 is merged, can we make progress here?

gurjotkaur20 · 2024-02-07T01:45:15Z

Sure.

Consolidating the changes required:

Add stream creation timestamp creation_time in the stats api.
Update manifest list in snapshot during retention cleanup. Currently is it not getting updated.
Once retention cleanup is done, get the p_timestamp from the first file of manifest list in snapshot and set it in the first_event_at in the stats api.

nitisht · 2024-02-11T10:28:48Z

Thanks @gurjotkaur20 we'll review this shortly

server/src/catalog.rs

nikhilsinhaparseable · 2024-02-13T12:11:29Z

@gurjotkaur20 when I ingest one event at a time using postman, the code works as expected
but when I try ingesting using Quest, I see below error -

thread 'thread 'actix-rt|system:0|arbiter:3' panicked at actix-rt|system:0|arbiter:0thread 'server/src/storage/object_storage.rs' panicked at actix-rt|system:0|arbiter:4:216server/src/storage/object_storage.rs::thread '216actix-rt|system:0|arbiter:5:' panicked at 53' panicked at server/src/storage/object_storage.rs:21653::
parseable config is valid json: Error("EOF while parsing a string", line: 1, column: 32):

parseable config is valid json: Error("EOF while parsing a value", line: 1, column: 0)server/src/storage/object_storage.rs:53note: run with RUST_BACKTRACE=1 environment variable to display a backtrace
216
:53:
parseable config is valid json: Error("EOF while parsing a string", line: 1, column: 32)
thread 'actix-rt|system:0|arbiter:1' panicked at server/src/storage/object_storage.rs:216::
parseable config is valid json: Error("EOF while parsing a value", line: 1, column: 0)
53:
parseable config is valid json: Error("EOF while parsing a value", line: 1, column: 0)

Steps -

Run parseable server (i ran with local-store)
Start ingesting with Quest
The ingestion fails after a few seconds
I see error in Parseable server console - thread 'actix-rt|system:0|arbiter:5' panicked at server/src/storage/object_storage.rs:216:53:
parseable config is valid json: Error("EOF while parsing a value", line: 1, column: 0)
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

Please check.
Do let me know if you need help in setting up Quest for ingestion.

gurjotkaur20 · 2024-02-15T13:00:08Z

@nikhilsinhacloudsurfex I have tried running quest in container as well as on local but didn't find this issue. Please help in reproducing if you have any particular scenario in mind.

server/src/handlers/http/ingest.rs

nitisht · 2024-02-18T10:49:28Z

server/src/storage/retention.rs

+            log::error!("Failed to update manifest list in the snapshot {err:?}")
+        }
+
+        if let Ok(Some(first_event_at)) = catalog::get_first_event(store, &stream_name).await {


Why do we need to do this here?

This is here to update the first event timestamp after cleanup because after retention cleanup, first event timestamp would be different.

nikhilsinhaparseable

approved, good to merge

Changes does in the PR - 1. adds the first_event_at property (from the min value of p_timestamp of the first parquet file listed in the first manifest file from the snapshot of the stream.json) to the stats api and writes it to the stream.json file at the request of get stats. 2. updates the first_event_at in case of retention Fixes : parseablehq#587

fix: add stream creation time in get stats api

adbae20

nitisht requested a review from nikhilsinhaparseable January 22, 2024 07:55

nitisht mentioned this pull request Jan 22, 2024

added a stream init time field #590

Closed

4 tasks

gurjotkaur20 force-pushed the add-stream-creation-time branch from b5d549d to adbae20 Compare January 23, 2024 16:14

gurjotkaur20 marked this pull request as draft January 24, 2024 02:14

nitisht requested changes Jan 25, 2024

View reviewed changes

gurjotkaur20 force-pushed the add-stream-creation-time branch from 8be084b to adbae20 Compare February 7, 2024 01:54

gurjotkaur20 and others added 5 commits February 7, 2024 07:24

Merge branch 'main' into add-stream-creation-time

9a5d4aa

Merge branch 'main' into add-stream-creation-time

01092c3

fix: adding first_event_at timestamp in the stats api

4c5228e

fix: cargo fmt check

56851f9

fix: cargo check

a4173b9

gurjotkaur20 marked this pull request as ready for review February 11, 2024 08:53

nikhilsinhaparseable reviewed Feb 13, 2024

View reviewed changes

server/src/catalog.rs Outdated Show resolved Hide resolved

fix: modify min to max

b808525

nitisht reviewed Feb 18, 2024

View reviewed changes

server/src/handlers/http/ingest.rs Outdated Show resolved Hide resolved

nitisht reviewed Feb 18, 2024

View reviewed changes

fix: move checking first_event_at_empty flow to stats api

58b3ee9

gurjotkaur20 requested a review from nitisht February 24, 2024 05:30

nikhilsinhaparseable approved these changes Feb 26, 2024

View reviewed changes

nikhilsinhaparseable merged commit e7cd586 into parseablehq:main Feb 26, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add stream creation time in get stats api #632

fix: add stream creation time in get stats api #632

gurjotkaur20 commented Jan 22, 2024 •

edited

Loading

nitisht commented Jan 22, 2024

gurjotkaur20 commented Jan 22, 2024

nitisht commented Jan 22, 2024

nitisht commented Jan 22, 2024 •

edited

Loading

logut commented Jan 22, 2024

nitisht commented Jan 22, 2024

gurjotkaur20 commented Jan 23, 2024

nitisht left a comment

nitisht commented Feb 6, 2024

gurjotkaur20 commented Feb 7, 2024 •

edited

Loading

nitisht commented Feb 11, 2024

nikhilsinhaparseable commented Feb 13, 2024

gurjotkaur20 commented Feb 15, 2024 •

edited

Loading

nitisht Feb 18, 2024

gurjotkaur20 Feb 18, 2024

nikhilsinhaparseable left a comment

fix: add stream creation time in get stats api #632

fix: add stream creation time in get stats api #632

Conversation

gurjotkaur20 commented Jan 22, 2024 • edited Loading

Description

nitisht commented Jan 22, 2024

gurjotkaur20 commented Jan 22, 2024

nitisht commented Jan 22, 2024

nitisht commented Jan 22, 2024 • edited Loading

logut commented Jan 22, 2024

nitisht commented Jan 22, 2024

gurjotkaur20 commented Jan 23, 2024

nitisht left a comment

Choose a reason for hiding this comment

nitisht commented Feb 6, 2024

gurjotkaur20 commented Feb 7, 2024 • edited Loading

nitisht commented Feb 11, 2024

nikhilsinhaparseable commented Feb 13, 2024

gurjotkaur20 commented Feb 15, 2024 • edited Loading

nitisht Feb 18, 2024

Choose a reason for hiding this comment

gurjotkaur20 Feb 18, 2024

Choose a reason for hiding this comment

nikhilsinhaparseable left a comment

Choose a reason for hiding this comment

gurjotkaur20 commented Jan 22, 2024 •

edited

Loading

nitisht commented Jan 22, 2024 •

edited

Loading

gurjotkaur20 commented Feb 7, 2024 •

edited

Loading

gurjotkaur20 commented Feb 15, 2024 •

edited

Loading